Skip to content

Make got an external dependency for the Node build target#15

Closed
joshlory wants to merge 1 commit intoscratchfoundation:developfrom
joshlory:make-got-external-dependency
Closed

Make got an external dependency for the Node build target#15
joshlory wants to merge 1 commit intoscratchfoundation:developfrom
joshlory:make-got-external-dependency

Conversation

@joshlory
Copy link
Copy Markdown

@joshlory joshlory commented Aug 4, 2017

Resolves

Issue #14.

Proposed Changes

Move got from a "devDependency" to a "dependency". Mark it as external for the Node.js webpack build.

Reason for Changes

Making got an explicit dependency lets the consuming project control whether got is built for a node or web target.

Test Coverage

I struggled a bit getting solid test coverage of this, or even a standalone repro case. It only happens when for import Storage from 'scratch-storage' is used in a client-side project.

I wrote a sample test case to verify gzipped assets can be loaded, but npm test runs via Node.js it doesn't repro the issue 😕. Happy to add appropriate coverage if you know of a better approach!

const crypto = require('crypto');
const test = require('tap').test;
const zlib = require('zlib');
const http = require('http');

global.self = {};
require('../../dist/web/scratch-storage');

const port = 8071;
const svgData = `<?xml version="1.0"?>
<svg width="100" height="100" xmlns="http://www.w3.org/2000/svg">
  <circle fill="#F00" cx="50" cy="50" r="30"/>
</svg>
`;

test('loadGzippedAsset', t => {
    const server = http.createServer((request, response) => {
        const raw = zlib.gzipSync(svgData);
        response.writeHead(200, {'Content-Encoding': 'gzip'});
        response.end(raw);
    }).listen(port);

    const storage = new self.Scratch.Storage();
    storage.addWebSource(
        [storage.AssetType.ImageVector],
        () => `http://localhost:${port}`
    );

    const assetType = storage.AssetType.ImageVector;
    const id = '8e768a5a5a01891b05c01c9ca15eb6aa';

    const promise = storage.load(assetType, id);
    t.type(promise, 'Promise');

    promise.then(asset => {
        t.type(asset, storage.Asset);
        t.strictEqual(asset.assetType, assetType);
        t.strictEqual(asset.assetId, id);

        const hash = crypto.createHash('md5');
        hash.update(asset.data);
        t.strictEqual(hash.digest('hex'), id);

        server.close();
    });

    return promise;
});

@joshlory
Copy link
Copy Markdown
Author

joshlory commented Aug 4, 2017

(I think this issue goes away if scratch-storage switches to nets along with scratch-vm, see: scratchfoundation/scratch-vm#585)

@thisandagain
Copy link
Copy Markdown
Contributor

Thoughts on this @rschamp @cwillisf? I think I'd rather resolve this issue by removing got and normalizing on use of the nets package.

@thisandagain thisandagain modified the milestones: Aug 23, Sept 20 Aug 10, 2017
@rschamp
Copy link
Copy Markdown
Contributor

rschamp commented Aug 10, 2017

+1 to moving to nets.

@joshlory
Copy link
Copy Markdown
Author

Closing this PR. Thanks!

@joshlory joshlory closed this Aug 10, 2017
@joshlory joshlory deleted the make-got-external-dependency branch August 10, 2017 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants